Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QuickLook] Update bindings for Xcode 13.0 beta 1,2,3,4 #12337

Merged
merged 5 commits into from
Aug 6, 2021

Conversation

rachelkang
Copy link
Contributor

Updates to this PR do not include updates relevant to QuickLookThumbnailing, QuickLookUI

Generator fix courtesy of @spouliot

[QuickLook] QLPreviewSceneActivationConfiguration not yet bound in Xcode13

@rachelkang rachelkang added the note-highlight Worth calling out specifically in release notes label Aug 3, 2021
@rachelkang rachelkang added this to the xcode13.0 milestone Aug 3, 2021
nint InitialPreviewIndex { get; set; }
}

// TODO: BaseType UIWindowSceneActivationConfiguration must first be implemented in UIKit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[QuickLook] QLPreviewSceneActivationConfiguration not yet bound in Xcode13 because base type UIWindowSceneActivationConfiguration is not yet bound in UIKit

src/quicklook.cs Outdated Show resolved Hide resolved
src/quicklook.cs Outdated Show resolved Hide resolved
src/quicklook.cs Outdated Show resolved Hide resolved
src/quicklook.cs Show resolved Hide resolved
{
}

[iOS (15,0), MacCatalyst (15,0)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mac ?

without a [Mac] it means the oldest supported (10,9) has it available.

Same for watchOS and tvOS. You likely want [NoWatch, NoTV, NoMac] if it's not available on macOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there's no indication on the diffs, xtro, web docs, or header files - I tend to leave the availability of unmentioned platforms as is. But is it best to assume that there is no availability on those platforms? Since it's a breaking change to remove things, and it's always easier to introduce things later on?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is it best to assume that there is no availability on those platforms?

No. Lack of availability attributes means it's been available since our earliest supported versions.
E.g. lack of [NoWatch] implies it's [Watch (2,0)] and that the API has been available in the past 6 years (or so)

This is almost always wrong when adding bindings for a new Xcode (unless it added a new platform).

Since it's a breaking change to remove things, and it's always easier to introduce things later on?

True :-) However it's true for code that we ship (post generation), not the binding sources!

Example: [NoWatch] -> means we're not generating the API on watchOS.

If you don't add [NoWatch] (in binding sources) then the API will be present inside Xamarin.WatchOS.dll (and assumed to be available since 2.0 which is watchOS minimum supported version).

If you want to fix that later, by adding [NoWatch], then you are creating a breaking change (since the API won't be part of Xamarin.WatchOS.dll in the next release).

Copy link
Contributor Author

@rachelkang rachelkang Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think my question was a bit confusing! I understand what you explained about the lack of availability attributes in our code - I meant instead to ask about how we should interpret the lack of mention of platform availability in diffs, xtro, web docs, and header files.

So in this case, there is no Watch availability or unavailability mentioned whatsoever in the diffs, xtro, web docs, or header files for this API. So I was asking if that means it's best to assume there's no watch availability and tag as [NoWatch] - does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short answer: Whenever in doubt it's better to assume [No*] if availability is not mentioned because it means no code will be generated (and it won't be a breaking change to add it later).

When you add (or modify) sharpie's output then it's good to add comments (for reviewers and people who will update the bindings... years later), e.g.

[NoWatch][NoTV][NoMac] // availability not mentioned in the header files

Long answer

  • Apple's macro are often incomplete or incorrect (e.g. wrong version)
    • Some (not all) gets fixed in newer betas or the next Xcode...
  • There are assumptions like
    • old (pre 9.0) iOS API are assumed to be available on tvOS 9 (first version) unless decorated otherwise
    • old (pre 13) iOS API are assumed to be available on MacCatalyst 13 (first version) ...
    • enums are not always decorated since they are not API (you can see which API uses them to know which attributes are needed)
  • Intro (runtime) and xtro (build time) will spot many, but not all, errors
  • over time you get to know the API/framework and can identify things that looks unusual (and add tests to confirm your gut feelings)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, thank you!!!

Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!!

src/quicklook.cs Outdated
interface QLFilePreviewRequest
{
[Export ("fileURL")]
NSUrl FileURL { get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
NSUrl FileURL { get; }
NSUrl FileUrl { get; }

Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bravo, approving taking into account sebs comments.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 91 tests passed.

Failed tests

  • monotouch-test/Mac Catalyst/Debug [dotnet]: Failed (Tests run: 2629 Passed: 2490 Inconclusive: 35 Failed: 1 Ignored: 138)

Pipeline on Agent XAMBOT-1094.BigSur'
Merge 6adb0c5 into 4aa51ba

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

1 tests failed, 91 tests passed.

Failed tests

  • Generator tests/.NET: Failed (Execution failed with exit code 1)

Pipeline on Agent XAMBOT-1096.BigSur'
Merge 75ac7eb into 0ffed97

@spouliot
Copy link
Contributor

spouliot commented Aug 6, 2021

@rachelkang it works locally for me and, if it was related to the generator change it should have failed in the non-botnet execution too. It would also have failed on the previous commits.

Logs shows

12:22:05.2558270   Failed NoWarn(macOSMobile) [12 s]
12:22:05.2558370   Error Message:
12:22:05.2558470    System.NullReferenceException : Object reference not set to an instance of an object.
12:22:05.2558570   Stack Trace:
12:22:05.2558660      at Xamarin.Tests.ThreadStaticTextWriter.WriteLine(String value) in /Users/builder/azdo/_work/1/s/xamarin-macios/tests/generator/BGenTool.cs:line 478
12:22:05.2558760    at System.IO.TextWriter.SyncTextWriter.WriteLine(Object value) in System.Private.CoreLib.dll:token 0x6005fe5+0x0
12:22:05.2558870    at System.Console.WriteLine(Object value) in System.Console.dll:token 0x6000082+0xb
12:22:05.2558970    at Xamarin.Tests.BGenTool.Execute() in /Users/builder/azdo/_work/1/s/xamarin-macios/tests/generator/BGenTool.cs:line 235
12:22:05.2559090    at Xamarin.Tests.BGenTool.AssertExecute(String message) in /Users/builder/azdo/_work/1/s/xamarin-macios/tests/generator/BGenTool.cs:line 207
12:22:05.2559320    at GeneratorTests.ErrorTests.NoWarn(Profile profile) in /Users/builder/azdo/_work/1/s/xamarin-macios/tests/generator/ErrorTests.cs:line 868

which is not in your code (but the test code) so it looks like a very uncommon random failure.

I file it inside maccore https://github.com/xamarin/maccore/issues/2489

You proceed with the merge!

@rachelkang
Copy link
Contributor Author

@rachelkang it works locally for me and, if it was related to the generator change it should have failed in the non-botnet execution too. It would also have failed on the previous commits.

Logs shows

12:22:05.2558270   Failed NoWarn(macOSMobile) [12 s]
12:22:05.2558370   Error Message:
12:22:05.2558470    System.NullReferenceException : Object reference not set to an instance of an object.
12:22:05.2558570   Stack Trace:
12:22:05.2558660      at Xamarin.Tests.ThreadStaticTextWriter.WriteLine(String value) in /Users/builder/azdo/_work/1/s/xamarin-macios/tests/generator/BGenTool.cs:line 478
12:22:05.2558760    at System.IO.TextWriter.SyncTextWriter.WriteLine(Object value) in System.Private.CoreLib.dll:token 0x6005fe5+0x0
12:22:05.2558870    at System.Console.WriteLine(Object value) in System.Console.dll:token 0x6000082+0xb
12:22:05.2558970    at Xamarin.Tests.BGenTool.Execute() in /Users/builder/azdo/_work/1/s/xamarin-macios/tests/generator/BGenTool.cs:line 235
12:22:05.2559090    at Xamarin.Tests.BGenTool.AssertExecute(String message) in /Users/builder/azdo/_work/1/s/xamarin-macios/tests/generator/BGenTool.cs:line 207
12:22:05.2559320    at GeneratorTests.ErrorTests.NoWarn(Profile profile) in /Users/builder/azdo/_work/1/s/xamarin-macios/tests/generator/ErrorTests.cs:line 868

which is not in your code (but the test code) so it looks like a very uncommon random failure.

I file it inside maccore xamarin/maccore#2489

You proceed with the merge!

awesome, thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
note-highlight Worth calling out specifically in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants